Skip to content

Conversation

@rocketchat-github-ci
Copy link
Collaborator

@rocketchat-github-ci rocketchat-github-ci commented Sep 17, 2025

Summary by CodeRabbit

  • Bug Fixes

    • Iframe-based login now auto-triggers when configured, with improved Electron URL handling and cleaner requests.
    • Server API consistently enforces authentication: unauthenticated requests now receive 401 with "You must be logged in to do this."
    • Slash-command update handling refined to avoid reacting to disabled events.
  • New Features

    • App lifecycle can opt to preserve slash commands during purge/disable flows; command execution now surfaces errors when apps are missing or disabled.
    • Custom sound volume is now clamped and normalized (0–1) for consistent playback.
  • Tests

    • End-to-end and unit tests updated to reflect auth, slash-command, and volume behavior.
  • Chores

    • Release metadata changes and security hotfix notes added.

You can see below a preview of the release change log:

7.10.1

Engine versions

  • Node: 22.16.0
  • Deno: 1.43.5
  • MongoDB: 5.0, 6.0, 7.0
  • Apps-Engine: 1.55.1

Patch Changes

@changeset-bot
Copy link

changeset-bot bot commented Sep 17, 2025

🦋 Changeset detected

Latest commit: b3ebad4

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 40 packages
Name Type
@rocket.chat/meteor Patch
@rocket.chat/apps-engine Patch
@rocket.chat/apps Patch
@rocket.chat/core-services Patch
@rocket.chat/core-typings Patch
@rocket.chat/fuselage-ui-kit Patch
@rocket.chat/rest-typings Patch
@rocket.chat/ddp-streamer Patch
@rocket.chat/presence Patch
rocketchat-services Patch
@rocket.chat/uikit-playground Patch
@rocket.chat/api-client Patch
@rocket.chat/cron Patch
@rocket.chat/ddp-client Patch
@rocket.chat/freeswitch Patch
@rocket.chat/gazzodown Patch
@rocket.chat/http-router Patch
@rocket.chat/livechat Patch
@rocket.chat/model-typings Patch
@rocket.chat/ui-avatar Patch
@rocket.chat/ui-client Patch
@rocket.chat/ui-contexts Patch
@rocket.chat/web-ui-registration Patch
@rocket.chat/account-service Patch
@rocket.chat/authorization-service Patch
@rocket.chat/omnichannel-transcript Patch
@rocket.chat/presence-service Patch
@rocket.chat/queue-worker Patch
@rocket.chat/stream-hub-service Patch
@rocket.chat/license Patch
@rocket.chat/omnichannel-services Patch
@rocket.chat/pdf-worker Patch
@rocket.chat/models Patch
@rocket.chat/network-broker Patch
@rocket.chat/mock-providers Patch
@rocket.chat/ui-video-conf Patch
@rocket.chat/ui-voip Patch
@rocket.chat/instance-status Patch
@rocket.chat/omni-core Patch
@rocket.chat/omni-core-ee Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@dionisio-bot
Copy link
Contributor

dionisio-bot bot commented Sep 17, 2025

Looks like this PR is not ready to merge, because of the following issues:

  • This PR is missing the 'stat: QA assured' label
  • This PR is missing the required milestone or project

Please fix the issues and try again

If you have any trouble, please check the PR guidelines

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Sep 17, 2025

Walkthrough

Tightens server API auth to return 401 for unauthenticated reads; updates related end-to-end tests; enhances client iframe login hook with gating, auto-trigger, and Electron handling; extends mock providers with streaming support and re-exports; adjusts apps-engine slash-command purge/execute behaviors; moves and tests sound volume formatting; adds multiple changeset files.

Changes

Cohort / File(s) Summary of edits
Release metadata / changesets
.changeset/bump-patch-1758105326594.md, .changeset/green-ants-shop.md, .changeset/grumpy-berries-arrive.md, .changeset/old-meals-pull.md, .changeset/spicy-crabs-complain.md
Added multiple changeset files describing patch/security releases and notes for @rocket.chat/meteor / @rocket.chat/apps-engine. No runtime or API-signature changes.
Client hook — iframe login
apps/meteor/client/hooks/iframe/useIframe.ts
Added local enabled gating, early-exit in tryLogin, auto-invoke via useEffect, Electron client=electron query handling, removed explicit Content-Type header, exposed enabled in return and adjusted deps.
Server API — auth guard
apps/meteor/app/api/server/ApiClass.ts
Derives user from authenticated session; introduces shouldPreventAnonymousRead / shouldPreventUserRead guards; returns 401 with "You must be logged in to do this." and legacy-compatible body on unauthorized requests.
End-to-end tests
apps/meteor/tests/end-to-end/api/channels.ts, apps/meteor/tests/end-to-end/api/users.ts
Updated expectations to 401 and new message for unauthenticated reads; added /users.info auth tests and a suite toggling Accounts_AllowAnonymousRead.
App slash commands — client & tests
apps/meteor/client/hooks/useAppSlashCommands.ts, apps/meteor/client/hooks/useAppSlashCommands.spec.tsx
Removed command/disabled invalidation handling; added/updated tests validating fetch and stream-driven add/update/remove behaviors.
Apps engine — purge/enable/update flows
packages/apps-engine/src/server/AppManager.ts
Added keepSlashcommands purge option; conditional purge/unregister logic for slash commands across disable/update/init/enable flows; adjusted control flow and registration timing.
App slash command execution
packages/apps-engine/src/server/managers/AppSlashCommandManager.ts, packages/apps-engine/tests/server/managers/AppSlashCommandManager.spec.ts
executeCommand now throws when app missing or not enabled (instead of silent return); updated test to expect a thrown error for a failing command.
Mock providers — streaming support & re-export
packages/mock-providers/src/MockedAppRootBuilder.tsx, packages/mock-providers/src/index.ts
Imported streaming types; added ExtractKeys/AllStreamerEventKeys and exported StreamControllerRef type; added withStream<N>(...) builder API to wire stream controllers; re-exported builder from package index.
CustomSoundProvider — formatVolume refactor & tests
apps/meteor/client/providers/CustomSoundProvider/lib/formatVolume.ts, apps/meteor/client/providers/CustomSoundProvider/lib/formatVolume.spec.ts, apps/meteor/client/providers/CustomSoundProvider/lib/index.ts, apps/meteor/client/providers/CustomSoundProvider/lib/helpers.ts, apps/meteor/client/providers/CustomSoundProvider/CustomSoundProvider.tsx
Extracted formatVolume into its own file with clamping to [0,1]; added unit tests; added lib barrel exporting helpers and formatVolume; removed formatVolume from old helpers and updated import in provider to use ./lib.

Sequence Diagram(s)

sequenceDiagram
    participant Client as Browser/Electron Client
    participant Hook as useIframe Hook
    participant IframeAPI as iframe auth endpoint
    Note over Hook: useEffect may call tryLogin when enabled
    Client->>Hook: mount / call hook
    Hook->>Hook: compute enabled = Boolean(iframeEnabled && accountIframeUrl && apiUrl && apiMethod)
    alt enabled == false
        Hook-->>Client: no-op (tryLogin exits)
    else enabled == true
        Hook->>Hook: build iframeLoginUrl (append client=electron if Electron)
        Hook->>IframeAPI: fetch(iframeLoginUrl) (headers: undefined)
        IframeAPI-->>Hook: response (token / error)
        Hook->>Client: loginWithToken / iframe login flow
    end
Loading
sequenceDiagram
    participant Requester as External Client
    participant ApiClass as apps/meteor ApiClass handler
    participant Auth as Accounts/session
    Note over ApiClass: Guards check authenticated user and flags before handling
    Requester->>ApiClass: HTTP request (may include headers)
    ApiClass->>Auth: resolve current user
    alt no user & authOrAnonRequired && !allowAnonymousRead
        ApiClass-->>Requester: 401 "You must be logged in to do this." (legacy body if needed)
    else no user & authRequired
        ApiClass-->>Requester: 401 "You must be logged in to do this." (legacy body if needed)
    else
        ApiClass->>ApiClass: continue rate-limit, params, permissions, handler
        ApiClass-->>Requester: 200/other response
    end
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

Suggested labels

stat: ready to merge, stat: QA assured

Suggested reviewers

  • rodrigok
  • scuciatto

Poem

I twitch my whiskers, patch in paw,
I gate the login and add electron's law.
Streams hum "event" and tests hop along,
Servers say "401" — the guard stays strong.
I nibble a carrot — version bumped, hooray! 🥕

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Title Check ✅ Passed The title “Release 7.10.1” accurately reflects the primary purpose of the changeset, which is to bump the project to version 7.10.1. It is concise, specific, and follows standard conventions for release PRs, making it clear to reviewers and maintainers what this pull request achieves.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch release-7.10.1

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
.changeset/bump-patch-1758105326594.md (1)

5-5: Make the changelog line more descriptive

Consider including the release number and intent for clearer generated notes.

-Bump @rocket.chat/meteor version.
+Release 7.10.1: bump @rocket.chat/meteor patch version.
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

  • Jira integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 0aa2f45 and cee7038.

⛔ Files ignored due to path filters (1)
  • yarn.lock is excluded by !**/yarn.lock, !**/*.lock
📒 Files selected for processing (1)
  • .changeset/bump-patch-1758105326594.md (1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
  • GitHub Check: 📦 Build Packages
  • GitHub Check: CodeQL-Build
  • GitHub Check: update-pr
  • GitHub Check: CodeQL-Build
🔇 Additional comments (2)
.changeset/bump-patch-1758105326594.md (2)

1-3: LGTM: valid Changesets frontmatter

The package spec and release type are correctly formatted for Changesets.


1-5: Verify package name alignment and stray changeset

.changeset/bump-patch-1758105326594.md updates '@rocket.chat/meteor', but I could not confirm a package.json with "name": "@rocket.chat/meteor" (scan hit read/permission errors). Confirm a workspace/package exists with that exact name and that no other changesets unintentionally bump the same package.

@codecov
Copy link

codecov bot commented Sep 17, 2025

Codecov Report

❌ Patch coverage is 90.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 66.19%. Comparing base (0aa2f45) to head (b3ebad4).
⚠️ Report is 9 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #36965      +/-   ##
==========================================
+ Coverage   66.12%   66.19%   +0.06%     
==========================================
  Files        3287     3289       +2     
  Lines      110194   110356     +162     
  Branches    20930    20907      -23     
==========================================
+ Hits        72869    73045     +176     
+ Misses      34644    34627      -17     
- Partials     2681     2684       +3     
Flag Coverage Δ
e2e 57.38% <77.77%> (-0.03%) ⬇️
unit 71.51% <100.00%> (+0.07%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@kodiakhq kodiakhq bot requested a review from a team as a code owner September 17, 2025 14:25
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
apps/meteor/client/hooks/iframe/useIframe.ts (1)

50-60: Fix double-read of Response body and add error handling; remove redundant header.

  • result.json() is called twice (Line 62 and Line 68) — the second call will reject because the body is already consumed.
  • fetch errors (network, CORS, etc.) are not caught, leading to unhandled rejections.
  • headers: undefined is unnecessary; omit the field.

Apply this consolidated patch:

       const result = await fetch(apiUrl, {
         method: apiMethod,
-        headers: undefined,
         credentials: 'include',
       });
 
-      if (!result.ok || result.status !== 200) {
+      if (!result.ok || result.status !== 200) {
         setIframeLoginUrl(url);
-        callback?.(new Error(), null);
+        callback?.(new Error(`Iframe auth ${apiMethod} ${apiUrl} failed with ${result.status}`), null);
         return;
       }
 
-      loginWithToken(await result.json(), async (error: Meteor.Error | Meteor.TypedError | Error | null | undefined) => {
+      const tokenData = await result.json();
+      loginWithToken(tokenData, (error: Meteor.Error | Meteor.TypedError | Error | null | undefined) => {
         if (error) {
           setIframeLoginUrl(url);
         } else {
           setIframeLoginUrl(undefined);
         }
-        callback?.(error, await result.json());
+        callback?.(error, tokenData);
       });

Optionally, wrap the whole fetch/login block in a try/catch to handle network exceptions:

-      const result = await fetch(apiUrl, {
+      try {
+        const result = await fetch(apiUrl, {
           method: apiMethod,
           credentials: 'include',
-      });
+        });-      loginWithToken(tokenData, (error: Meteor.Error | Meteor.TypedError | Error | null | undefined) => {
+      loginWithToken(tokenData, (error: Meteor.Error | Meteor.TypedError | Error | null | undefined) => {
         …
       });
+      } catch (e) {
+        setIframeLoginUrl(url);
+        callback?.(e as Error, null);
+      }

Also applies to: 62-69

🧹 Nitpick comments (2)
apps/meteor/client/hooks/iframe/useIframe.ts (2)

15-16: Harden the enablement check (guard against whitespace-only values).

Trim string settings to avoid accidental truthiness when values contain only whitespace.

-const enabled = Boolean(iframeEnabled && accountIframeUrl && apiUrl && apiMethod);
+const enabled = Boolean(
+  iframeEnabled &&
+  accountIframeUrl?.trim() &&
+  apiUrl?.trim() &&
+  apiMethod?.trim()
+);

74-77: Optional: cancel in-flight request on unmount.

Consider wiring an AbortController and aborting in the effect cleanup to avoid setting state after unmount.

Would you like a follow-up patch adding AbortController and effect cleanup?

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

  • Jira integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between cee7038 and d7da2c0.

📒 Files selected for processing (2)
  • .changeset/green-ants-shop.md (1 hunks)
  • apps/meteor/client/hooks/iframe/useIframe.ts (5 hunks)
✅ Files skipped from review due to trivial changes (1)
  • .changeset/green-ants-shop.md
🧰 Additional context used
🧬 Code graph analysis (1)
apps/meteor/client/hooks/iframe/useIframe.ts (1)
apps/meteor/tests/data/api-data.ts (1)
  • apiUrl (8-8)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
  • GitHub Check: 🔎 Code Check / TypeScript
  • GitHub Check: 🔨 Test Unit / Unit Tests
  • GitHub Check: 🔨 Test Storybook / Test Storybook
  • GitHub Check: 🔎 Code Check / Code Lint
  • GitHub Check: 📦 Meteor Build - coverage
  • GitHub Check: CodeQL-Build
  • GitHub Check: CodeQL-Build
🔇 Additional comments (4)
apps/meteor/client/hooks/iframe/useIframe.ts (4)

2-2: LGTM: import addition is appropriate.

useEffect is required for the mount trigger you added.


36-38: LGTM: early exit guard.

Prevents unnecessary work and network calls when disabled.


71-71: LGTM: dependency array includes enabled.

Covers re-computation when settings flip.


79-79: LGTM: exposing enabled in the return API.

Makes gating state observable by callers.

Signed-off-by: Abhinav Kumar <[email protected]>
Co-authored-by: Julio Araujo <[email protected]>
Co-authored-by: Abhinav Kumar <[email protected]>
@julio-rocketchat julio-rocketchat requested a review from a team as a code owner September 19, 2025 19:15
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (2)
apps/meteor/app/api/server/ApiClass.ts (1)

819-823: 401 gating logic reads well; tighten typing on settings.get

Behavior matches the new contract, but add an explicit boolean generic to avoid truthy/falsey pitfalls from untyped settings.

Apply this diff:

-const shouldPreventAnonymousRead = !this.user && options.authOrAnonRequired && !settings.get('Accounts_AllowAnonymousRead');
+const shouldPreventAnonymousRead =
+  !this.user && options.authOrAnonRequired && !settings.get<boolean>('Accounts_AllowAnonymousRead');
.changeset/grumpy-berries-arrive.md (1)

5-5: Fix markdownlint: avoid bare URL

Wrap the URL to satisfy MD034 and improve readability.

Apply this diff:

-Security Hotfix (https://docs.rocket.chat/docs/security-fixes-and-updates)
+Security Hotfix ([Security fixes and updates](https://docs.rocket.chat/docs/security-fixes-and-updates))
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

  • Jira integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between d7da2c0 and c8e778a.

📒 Files selected for processing (4)
  • .changeset/grumpy-berries-arrive.md (1 hunks)
  • apps/meteor/app/api/server/ApiClass.ts (1 hunks)
  • apps/meteor/tests/end-to-end/api/channels.ts (1 hunks)
  • apps/meteor/tests/end-to-end/api/users.ts (1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
PR: RocketChat/Rocket.Chat#0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use descriptive test names that clearly communicate expected behavior

Applied to files:

  • apps/meteor/tests/end-to-end/api/channels.ts
🧬 Code graph analysis (1)
apps/meteor/tests/end-to-end/api/users.ts (1)
apps/meteor/tests/data/api-data.ts (1)
  • credentials (39-42)
🪛 markdownlint-cli2 (0.17.2)
.changeset/grumpy-berries-arrive.md

5-5: Bare URL used

(MD034, no-bare-urls)

🔇 Additional comments (3)
apps/meteor/tests/end-to-end/api/users.ts (2)

684-697: LGTM: adds unauthenticated guard for /users.info

Status 401 and minimal body checks are correct and stable.


698-729: LGTM: Anonymous Read enabled still requires auth for /users.info

Both cases (no headers, only X-User-Id) correctly expect 401.

apps/meteor/tests/end-to-end/api/channels.ts (1)

3506-3510: Loosen assertion on exact error text to reduce brittleness

Assert a substring or the errorType instead of the full message.

Location: apps/meteor/tests/end-to-end/api/channels.ts (around lines 3506–3510)

-          expect(res.body).to.have.a.property('error', 'You must be logged in to do this.');
+          expect(res.body).to.have.a.property('error').that.includes('logged in');

Search shows many tests assert exact error strings (e.g., 'error-not-allowed'); consider normalizing similar assertions or preferring errorType where present.

Comment on lines 811 to 817
if (options.authRequired || options.authOrAnonRequired) {
const user = await api.authenticatedRoute.call(this, this.request);
this.user = user!;
this.userId = String(this.request.headers.get('x-user-id'));
this.userId = this.user?._id;
const authToken = this.request.headers.get('x-auth-token');
this.token = (authToken && Accounts._hashLoginToken(String(authToken)))!;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

🧩 Analysis chain

Good change: derive userId from the authenticated user

Setting this.userId = this.user?._id prevents header spoofing from leaking into permission checks. LGTM.

Run to spot any lingering server code that still trusts x-user-id directly:

As a follow-up, consider eliminating x-user-id dependency during authentication entirely (derive the user solely from the token). Example refactor for authenticatedRoute:

// token-only lookup; avoids trusting a client-supplied user id
protected async authenticatedRoute(req: Request): Promise<IUser | null> {
  const authToken = req.headers.get('x-auth-token');
  if (!authToken) return null;

  const hashed = Accounts._hashLoginToken(String(authToken));
  return Users.findOne(
    { 'services.resume.loginTokens.hashedToken': hashed },
    { projection: getDefaultUserFields() },
  );
}

🏁 Script executed:

#!/bin/bash
rg -nP -C2 --type=ts "(x-user-id|X-User-Id)" apps/meteor | sed 's/^/apps\/meteor\//'

Length of output: 49525


Do not trust X-User-Id headers — remove/replace remaining server-side usages

Good change to derive this.userId from the authenticated user (prevents header spoofing). However the repo still contains server-side code that reads/trusts X-User-Id; these must be refactored to derive the user from the token (token-only lookup) before merging. Representative locations to fix:

  • apps/meteor/server/routes/userDataDownload.ts: req.headers['x-user-id'] (line ~36).
  • apps/meteor/app/api/server/middlewares/authentication.ts: copies/uses rc_uid / x-user-id from cookies/headers (lines ~22–27).
  • apps/meteor/app/api/server/helpers/getLoggedInUser.ts: request.headers.get('x-user-id') (lines ~6–9).
  • apps/meteor/app/api/server/ApiClass.ts: const { 'x-user-id': userId } = headers (lines ~964–965).
  • apps/meteor/app/file-upload/server/lib/FileUpload.ts: const uid = rc_uid || headers['x-user-id'] (line ~470).
  • Logging/CORS/docs: app/api/server/middlewares/logger.ts logs x-user-id; cors.ts and default/openApi.ts still advertise/allow X-User-Id — update accordingly.
  • Many end-to-end / e2e tests and fixtures under apps/meteor/tests/** still set X-User-Id — update tests or test helpers to use token-derived user ids.

Action: implement token-only authenticatedRoute (or equivalent) and remove header-based authorization/readers, update middleware/helpers/CORS/docs and tests. Block merge until server-side reliance on client-supplied X-User-Id is removed or explicitly validated.

🤖 Prompt for AI Agents
In apps/meteor/app/api/server/ApiClass.ts around lines 811 to 817, the code
still reads and trusts client-supplied headers (x-user-id) when setting
userId/token; remove any reliance on X-User-Id here and throughout the server.
Replace header-derived user identification with a token-only lookup: call the
authenticatedRoute (or equivalent token-validation routine) to resolve the user
object and set this.user and this.userId from that object only, derive
this.token from the x-auth-token after validating it server-side, and delete any
code that reads or copies rc_uid / x-user-id. Then audit and update the other
listed files (apps/meteor/server/routes/userDataDownload.ts,
app/api/server/middlewares/authentication.ts,
app/api/server/helpers/getLoggedInUser.ts, file-upload server code,
logger/CORS/docs) to remove header-based auth, update CORS/docs to no longer
advertise X-User-Id, and adjust tests/fixtures to use token-derived
authentication; do not merge until all server-side usages of X-User-Id are
removed or replaced with token-validated user lookups.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (4)
packages/mock-providers/src/MockedAppRootBuilder.tsx (4)

64-67: Remove commented‑out legacy interface.

Keeps the file clean and avoids confusion with the new typing approach.

-// interface MockedAppRootEvents extends Record<`stream-${StreamNames}-${StreamKeys<StreamNames>}`, any> {
-// 	'update-modal': void;
-// }

83-86: Consider stronger event arg typing (optional).

Record<AllStreamerEventKeys, any> sacrifices callback arg types. If Emitter supports tuple arg typing, you could map each key to its StreamerCallbackArgs tuple for safer .on/.emit usage. Not required for this PR.


87-92: Align StreamControllerRef signatures with stream callbacks.

Use rest parameters for emit and keep has parameter typed to StreamKeys<N> for better ergonomics and type safety.

-export type StreamControllerRef<N extends StreamNames> = {
-	controller?: {
-		emit: <K extends StreamKeys<N>>(eventName: K, args: StreamerCallbackArgs<N, K>) => void;
-		has: (eventName: StreamKeys<N>) => boolean;
-	};
-};
+export type StreamControllerRef<N extends StreamNames> = {
+	controller?: {
+		emit: <K extends StreamKeys<N>>(eventName: K, ...args: StreamerCallbackArgs<N, K>) => void;
+		has: <K extends StreamKeys<N>>(eventName: K) => boolean;
+	};
+};

286-308: withStream: pass options through, match emit/has types, and avoid string param.

Minor fidelity/type‑safety gaps: the wrapper drops getStream options, emit takes a tuple param instead of varargs, and has accepts string. Adjust as below.

-	withStream<N extends StreamNames>(streamName: N, ref: StreamControllerRef<N>): this {
+	withStream<N extends StreamNames>(streamName: N, ref: StreamControllerRef<N>): this {
 		const innerFn = this.server.getStream;
 
-		const outerFn: ServerContextValue['getStream'] = (innerStreamName) => {
+		const outerFn: ServerContextValue['getStream'] = (innerStreamName, _options) => {
 			if (innerStreamName === (streamName as StreamNames)) {
 				ref.controller = {
-					emit: <K extends StreamKeys<N>>(eventName: K, args: StreamerCallbackArgs<N, K>) => {
-						this.events.emit(`stream-${innerStreamName}-${eventName}` as AllStreamerEventKeys, ...args);
-					},
-					has: (eventName: string) => this.events.has(`stream-${innerStreamName}-${eventName}` as AllStreamerEventKeys),
+					emit: <K extends StreamKeys<N>>(eventName: K, ...args: StreamerCallbackArgs<N, K>) => {
+						this.events.emit(`stream-${innerStreamName}-${eventName}` as AllStreamerEventKeys, ...args);
+					},
+					has: <K extends StreamKeys<N>>(eventName: K) =>
+						this.events.has(`stream-${innerStreamName}-${eventName}` as AllStreamerEventKeys),
 				};
 
 				return <K extends StreamKeys<N>>(eventName: K, callback: StreamerCallback<N, K>) =>
 					this.events.on(`stream-${innerStreamName}-${eventName}` as AllStreamerEventKeys, callback);
 			}
 
-			return innerFn(innerStreamName);
+			return innerFn(innerStreamName, _options);
 		};
 
 		this.server.getStream = outerFn;
 
 		return this;
 	}

Please confirm @rocket.chat/emitter exposes has(eventName) returning boolean. If not, we should drop has or adapt it to listenerCount(eventName) > 0.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

  • Jira integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between c8e778a and b0c3e6a.

📒 Files selected for processing (2)
  • packages/mock-providers/src/MockedAppRootBuilder.tsx (3 hunks)
  • packages/mock-providers/src/index.ts (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/mock-providers/src/MockedAppRootBuilder.tsx (2)
packages/ddp-client/src/types/streams.ts (5)
  • StreamerEvents (34-574)
  • StreamNames (576-576)
  • StreamKeys (578-578)
  • StreamerCallbackArgs (585-590)
  • StreamerCallback (592-592)
packages/ui-contexts/src/ServerContext.ts (1)
  • ServerContextValue (19-53)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
  • GitHub Check: 📦 Build Packages
  • GitHub Check: update-pr
  • GitHub Check: CodeQL-Build
  • GitHub Check: CodeQL-Build
🔇 Additional comments (4)
packages/mock-providers/src/index.ts (1)

12-12: Public API surface expansion looks fine; confirm semver and release notes.

Re‑exporting MockedAppRootBuilder widens the package API. Ensure this is reflected in the changeset/release notes and is acceptable for 7.10.1 (patch) vs. minor.

packages/mock-providers/src/MockedAppRootBuilder.tsx (3)

12-21: Type‑only imports are correct.

Good split of DDP stream types; no runtime cost.


68-77: Template‑literal key extraction LGTM.

Nicely filters out widened string keys to preserve strong typing.


78-82: Union over all stream event keys LGTM.

Type composition is sound and scalable.

@kodiakhq kodiakhq bot requested a review from a team as a code owner September 23, 2025 13:20
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (6)
apps/meteor/client/hooks/useAppSlashCommands.ts (1)

17-25: Include queryClient in useDebouncedCallback dependencies to avoid stale closure.

queryClient is captured but deps are empty. It’s stable in practice, but adding it is safer.

-    100,
-    [],
+    100,
+    [queryClient],
packages/apps-engine/src/server/managers/AppSlashCommandManager.ts (1)

336-342: Good shift to explicit errors; include command context in messages.

Adding the command name improves debuggability without changing flow.

Apply:

-		if (!app) {
-			throw new Error('App not found');
-		}
-
-		if (!AppStatusUtils.isEnabled(await app.getStatus())) {
-			throw new Error('App not enabled');
+		if (!app) {
+			throw new Error(`App not found for command "${cmd}"`);
+		}
+
+		if (!AppStatusUtils.isEnabled(await app.getStatus())) {
+			throw new Error(`App not enabled for command "${cmd}"`);
 		}
packages/apps-engine/tests/server/managers/AppSlashCommandManager.spec.ts (1)

410-410: Tighten expectation by asserting the error message.

Validates the contract and guards against regressions.

Apply:

-		await Expect(() => ascm.executeCommand('failure', context)).toThrowAsync();
+		await Expect(() => ascm.executeCommand('failure', context)).toThrowErrorAsync(Error, /App not (found|enabled)/);
packages/apps-engine/src/server/AppManager.ts (3)

495-496: Risk of “ghost” slash commands while app is disabled.

Keeping slash commands registered during disable can surface commands to users that will now throw “App not enabled” at execution. Consider disabling commands at the bridge level while preserving in‑memory state.

Suggestion: add an AppSlashCommandManager API (e.g., disableCommandsForApp(appId)) that iterates touched/provided commands and calls bridge.doDisableCommand for each when keepSlashcommands is true in purge.


1054-1055: Registering commands during initialize may expose commands before enable.

This changes prior behavior and, combined with keepSlashcommands, increases the surface of “ghost” commands. Prefer registering on successful enable.

Apply:

-			await this.commandManager.registerCommands(app.getID());

And in enableApp (after successful enable, before releasing events):

 		if (enable) {
+			await this.commandManager.registerCommands(app.getID());
 			this.externalComponentManager.registerExternalComponents(app.getID());
 			await this.apiManager.registerApis(app.getID());
 			this.listenerManager.registerListeners(app);

1093-1096: If keeping slash commands, consider disabling them at the bridge.

Prevents user-visible commands from being runnable while app is disabled, without losing registration state.

Example (using a hypothetical manager API):

if (!opts.keepSlashcommands) {
  await this.commandManager.unregisterCommands(app.getID());
} else {
  await this.commandManager.disableCommandsForApp?.(app.getID());
}
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

  • Jira integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between b0c3e6a and 42cea25.

📒 Files selected for processing (6)
  • .changeset/old-meals-pull.md (1 hunks)
  • apps/meteor/client/hooks/useAppSlashCommands.spec.tsx (1 hunks)
  • apps/meteor/client/hooks/useAppSlashCommands.ts (1 hunks)
  • packages/apps-engine/src/server/AppManager.ts (6 hunks)
  • packages/apps-engine/src/server/managers/AppSlashCommandManager.ts (1 hunks)
  • packages/apps-engine/tests/server/managers/AppSlashCommandManager.spec.ts (1 hunks)
✅ Files skipped from review due to trivial changes (2)
  • apps/meteor/client/hooks/useAppSlashCommands.spec.tsx
  • .changeset/old-meals-pull.md
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
PR: RocketChat/Rocket.Chat#0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use descriptive test names that clearly communicate expected behavior

Applied to files:

  • packages/apps-engine/tests/server/managers/AppSlashCommandManager.spec.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
  • GitHub Check: 📦 Build Packages
  • GitHub Check: update-pr
  • GitHub Check: CodeQL-Build
  • GitHub Check: CodeQL-Build
🔇 Additional comments (3)
apps/meteor/client/hooks/useAppSlashCommands.ts (1)

31-38: Do not treat command/disabled as removed — tests and server expect it to be ignored.

There’s an explicit test apps/meteor/client/hooks/useAppSlashCommands.spec.tsx ("should ignore command/disabled event") that emits ['command/disabled', ['/test']] and asserts the command remains; the server emits this event (apps/meteor/server/modules/listeners/listeners.module.ts) and types declare it (packages/ddp-client/src/types/streams.ts). Changing behavior as suggested would break tests/contract — if you want to remove disabled commands, update tests and coordinate server/client behavior.

Likely an incorrect or invalid review comment.

packages/apps-engine/src/server/AppManager.ts (2)

829-831: LGTM: Don’t keep slash commands on update.

Matches the comment and avoids stale registrations across versions.


1179-1180: Same “ghost commands” concern when enable fails.

See comment on Line 495; consider disabling commands at the bridge when purging with keepSlashcommands: true.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (2)
apps/meteor/client/providers/CustomSoundProvider/lib/formatVolume.ts (1)

1-4: LGTM! Consider simplifying the precision handling.

The clamping and normalization logic is correct. The function properly handles out-of-range inputs and converts the 0-100 scale to the 0-1 range needed for audio volume.

However, the toPrecision(2) followed by Number() conversion seems overly complex for this use case. A simpler approach would be to return the normalized value directly:

 export const formatVolume = (volume: number) => {
 	const clamped = Math.max(0, Math.min(volume, 100));
-	return Number((clamped / 100).toPrecision(2));
+	return clamped / 100;
 };

This achieves the same result with clearer intent and avoids the string conversion roundtrip.

apps/meteor/client/providers/CustomSoundProvider/lib/formatVolume.spec.ts (1)

3-19: LGTM! Core functionality is well tested.

The test suite covers the essential cases: upper/lower bound clamping and mid-range normalization. The tests are clear and concise.

Optionally, consider adding tests for additional edge cases:

it('returns 0 if volume is 0', () => {
  expect(formatVolume(0)).toBe(0);
});

it('handles decimal inputs correctly', () => {
  expect(formatVolume(50.5)).toBe(0.505);
});

it('returns 0.01 if volume is 1', () => {
  expect(formatVolume(1)).toBe(0.01);
});
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

  • Jira integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 5e3fd35 and b3ebad4.

📒 Files selected for processing (6)
  • .changeset/spicy-crabs-complain.md (1 hunks)
  • apps/meteor/client/providers/CustomSoundProvider/CustomSoundProvider.tsx (1 hunks)
  • apps/meteor/client/providers/CustomSoundProvider/lib/formatVolume.spec.ts (1 hunks)
  • apps/meteor/client/providers/CustomSoundProvider/lib/formatVolume.ts (1 hunks)
  • apps/meteor/client/providers/CustomSoundProvider/lib/helpers.ts (0 hunks)
  • apps/meteor/client/providers/CustomSoundProvider/lib/index.ts (1 hunks)
💤 Files with no reviewable changes (1)
  • apps/meteor/client/providers/CustomSoundProvider/lib/helpers.ts
✅ Files skipped from review due to trivial changes (2)
  • apps/meteor/client/providers/CustomSoundProvider/lib/index.ts
  • .changeset/spicy-crabs-complain.md
🧰 Additional context used
🧬 Code graph analysis (1)
apps/meteor/client/providers/CustomSoundProvider/lib/formatVolume.spec.ts (1)
apps/meteor/client/providers/CustomSoundProvider/lib/formatVolume.ts (1)
  • formatVolume (1-4)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
  • GitHub Check: 📦 Build Packages
  • GitHub Check: update-pr
  • GitHub Check: CodeQL-Build
  • GitHub Check: CodeQL-Build
🔇 Additional comments (1)
apps/meteor/client/providers/CustomSoundProvider/CustomSoundProvider.tsx (1)

7-7: LGTM!

The import path refactoring to use the barrel export is clean and aligns with the new module structure where formatVolume has been extracted to its own file and both utilities are re-exported through lib/index.ts.

@ggazzo ggazzo merged commit f26aae0 into master Oct 9, 2025
44 of 52 checks passed
@ggazzo ggazzo deleted the release-7.10.1 branch October 9, 2025 17:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants